Skip to content

remove _file#257

Closed
Fil wants to merge 11 commits intomainfrom
fil/remove-_file
Closed

remove _file#257
Fil wants to merge 11 commits intomainfrom
fil/remove-_file

Conversation

@Fil
Copy link
Copy Markdown
Contributor

@Fil Fil commented Nov 28, 2023

closes #203

(We can do _import in a follow-up PR.)

@Fil Fil requested review from mbostock and trebor November 28, 2023 18:55
@Fil Fil marked this pull request as draft November 28, 2023 18:56
Comment thread src/preview.ts
// This handles a static file.
try {
await access(path, constants.R_OK);
if ((await stat(path)).isFile()) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is new… otherwise javascript was detected as the javascript/ directory

@Fil Fil marked this pull request as ready for review November 28, 2023 19:01
Copy link
Copy Markdown
Contributor

@trebor trebor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the one warning otherwise this looks good to me.

Comment thread src/preview.ts Outdated
Comment thread src/preview.ts Outdated
Copy link
Copy Markdown
Contributor

@trebor trebor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

Comment thread src/javascript/fetches.ts Outdated
Comment thread src/preview.ts Outdated
Comment thread src/build.ts
for (const file of files) {
let sourcePath = join(sourceRoot, file);
const outputPath = join(outputRoot, "_file", file);
const outputPath = join(outputRoot, file);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we error if the file already exists? (Which shouldn’t have been possible before, but should now be possible if you have e.g. an index.html and an index.md.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not error, but we must definitely skip the file in that case.

Here are examples (admittedly fringe) where a file points to a generated page:

javascript.md:
<a href=javascript.html download>download this page</a>

or
<a href=javascript download>download this page</a>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now skipping both of these

Copy link
Copy Markdown
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. But please drop the access check and the TODO simplify comments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we name test files to match the source file that they are testing. It seems to me this should be in test/markdown-test.ts, and that the test should be named normalizePieceHtml, since that’s the imported function that is being tested. This is currently described as “file attachments” and “added” which doesn’t precisely indicate the function being tested.

const htmlStr = html`<img src="./test.png">`;
const expected = html`<img src="./_file/test.png">`;
const htmlStr = html`<img src="./test.png"><img src="test.png">`;
const expected = html`<span><img src="./test.png"><img src="./test.png"></span>`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this testing that file paths are normalized and not listed twice? This is a good thing to mention in the test description so that we remember the intent of this test going forward.

Comment thread src/build.ts
// Copy over the referenced files.
for (const file of files) {
const outputPath = join(outputRoot, file);
if (existsSync(outputPath) || existsSync(outputPath + ".html")) continue; // skip pages
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we always want to add .html to the path here? Shouldn’t that only apply in extension-less cases? I feel like we are doing too much work to support this extreme edge case of linking to the page itself to download source code. Can we just remove the .html check entirely please?

Comment thread src/markdown.ts
}

const SUPPORTED_PROPERTIES: readonly {query: string; src: "href" | "src" | "srcset"}[] = Object.freeze([
{query: "a[href]", src: "href"},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{query: "a[href]", src: "href"},
{query: "a[download][href]", src: "href"},

Seems to me that we shouldn’t promote links to file attachments unless they are explicitly marked as downloads.

Philosophically, I don’t expect this functionality to work transparently. Meaning: we have to document which elements get this special treatment, and users will have to read the documentation to know which elements are handled automatically and which aren’t — we can’t guarantee that this will work. The only way to guarantee that it will work is for the user to declare the file attachment statically using FileAttachment in JavaScript. For example, we’re not including object[data] in this list, which looks like this

<object
  type="video/mp4"
  data="path/flower.webm"
  width="600"
  height="140">
  <img src="path/image.jpg" alt="useful image description">
</object>

I don’t think we should try to get everything here, and I don’t think we should feel “forced” to removing /_file to support promotion of linked files via a[href] — though I’m not really sure I follow why that solves a problem. And I do want to add content hashing of file attachments #260, so I’m not sure it is solving something.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And come to think of it, a more practical and reliable solution would to be have a public folder where everything gets copied #169, which we can offer in addition to promoting source files (and generated files) through static analysis. So then if you want to ensure that something is including in dist even if it’s not detectable through static analysis, you just drop it into public.

Copy link
Copy Markdown
Contributor Author

@Fil Fil Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about limiting to a[download]; I mean, it's fine to add a[download] to the list, but my main motivation was more on the side of the [view source](page.md) pattern, which is otherwise a bit difficult to create with file attachments. I'll probably come back to it in some other way—on its own it doesn't warrant such a big change in any case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think the most useful incarnation of [view source](page.md) will be to link to the editable version on the Observable platform or GitHub etc, where people will not only be able to view the source but contribute changes.

@Fil
Copy link
Copy Markdown
Contributor Author

Fil commented Nov 29, 2023

There are several elements in this PR; I'll split them into smaller chunks (and maybe get back to removing _file later). One important thing will be to clarify the routing precedence.

@Fil Fil closed this Nov 29, 2023
@Fil Fil mentioned this pull request Nov 29, 2023
@Fil Fil deleted the fil/remove-_file branch December 5, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

remove /_file/?

3 participants